Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Router Service] routeWillChange & routeDidChange #17025

Merged
merged 4 commits into from
Oct 3, 2018
Merged

Conversation

chadhietala
Copy link
Contributor

@chadhietala chadhietala commented Oct 1, 2018

This adds the routeWillChange & routeDidChange events. It also exposes to and from fields on the transition object. These properties point to a RouteInfo object that looks like:

interface RouteInfo {
  name: string;
  localName: string;
  params: Dict<unknown>;
  paramNames: string[];
  queryParams: Dict<unknown>;
  parent: RouteInfo | null;
  child: RouteInfo | null;
  find((routeInfo: RouteInfo, i: number, routeInfos: RouteInfo[]) => boolean, thisArg: any): RouteInfo : undefined;
}

This also deprecates didTransition and willTransition.

This should land after #17007

@@ -148,7 +150,17 @@ export default class RouterService extends Service {
}
}

RouterService.reopen({
RouterService.reopen(Evented, {
init() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move the init up into the main class body, and leave the .reopen(Evented) here?

@@ -329,7 +329,7 @@ class Route extends EmberObject implements IRoute {
/**
@private

@method _reset
@method _internalReset
@since 1.7.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think this @since is also wrong

false,
{
id: 'deprecate-router-events',
until: '3.9.0',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be 4.0.0, didTransition and willTransition were public API's.

false,
{
id: 'deprecate-router-events',
until: '3.9.0',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

until: '4.0.0'

@@ -1793,4 +1823,37 @@ EmberRouter.reopen(Evented, {
}),
});

if (EMBER_ROUTING_ROUTER_SERVICE) {
if (ROUTER_EVENTS) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you collapse these two conditionals?

if (EMBER_ROUTING_ROUTER_SERVICE && ROUTER_EVENTS) {

@@ -257,6 +259,34 @@ class EmberRouter extends EmberObject {
return triggerEvent.bind(router)(routeInfos, ignoreFailure, name, args);
}

routeWillChange(transition: Transition) {
if (EMBER_ROUTING_ROUTER_SERVICE) {
router.trigger('routeWillChange', transition);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new routeWillChange / routeDidChange events need documentation. Can you add that to the meta issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the documentation for this. Not sure if its good enough though. I will add it to the list. Also need to document to and from fields on the transition. Need to also mark Transition as public.

@chadhietala chadhietala force-pushed the to-from branch 2 times, most recently from 5447d27 to 9e31a44 Compare October 3, 2018 13:43
This removes the FF around all the router service features that were
go'd earlier this year. It also reuses the same flag to guard the new
features that we are adding.
@chadhietala chadhietala merged commit 7b00f7b into master Oct 3, 2018
@chadhietala chadhietala deleted the to-from branch October 3, 2018 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants